Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: replace Node.js 18 with Node.js 20 #1920

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

dcroote
Copy link
Contributor

@dcroote dcroote commented Dec 6, 2023

Closes #1918, implements #1910, and reverts the temporary #1873

@dcroote
Copy link
Contributor Author

dcroote commented Dec 6, 2023

@Siegrift - looks like this can't be implemented without using yarn install --ignore-engines (source) because of the use of the ^18.14.0 engine in @api3/commons@0.5.0 here.

The CI build error is:

error @api3/commons@0.5.0: The engine "node" is incompatible with this module. Expected version "^18.14.0". Got "20.10.0"

When I did use yarn install --ignore-engines locally, it installed, built and passed unit tests fine.

@dcroote dcroote self-assigned this Dec 6, 2023
@Siegrift
Copy link
Contributor

Siegrift commented Dec 6, 2023

I didn't know package manager check this field transitively. I assumed it's only for development/production purposes. I don't see a big issue with using --ignore-engines though.

@andreogle
Copy link
Member

andreogle commented Dec 6, 2023

I'm not sure about using --ignore-engines. Doesn't this mean this Airnode is then using the built-in node:x modules with Node v20, that are essentially untested (?). I assume nothing significant changed between v18 and v20, but that's a guess.

Is it not a better plan to bump the Node version in @api3/commons and release a new version of that? At least then we have some idea that nothing obvious broke through these tests. I feel like a given release of @api3/commons should be responsible for & explicit about what Node.js versions it supports/doesn't support. Using --ignore-engines sets a bad precedent for other API3 projects imo

@Siegrift
Copy link
Contributor

Siegrift commented Dec 6, 2023

Yeah, that's probably better. I still dislike the following:

  • You will not be able to use commons in older node (e.g. v18) but that's arguably a feature
  • It is a bit bad that a dependency forces you to update your node version, but I think we can specify a range in the package.json as well (not sure we need to bother with this though).

@andreogle
Copy link
Member

Assuming there are breaking changes, then this is what major version bumps should typically be used for imo. i.e. commons v0.x supports Node 18 and commons v1.x supports Node 20. This is pretty much the standard process to my knowledge.

If there aren't any obvious breaking changes, the easier option would be to allow both v18 and v20 in commons v0.x (like you suggested).

I just don't really like us ignoring what commons says it supports

@Siegrift
Copy link
Contributor

Siegrift commented Dec 6, 2023

Assuming there are breaking changes, then this is what major version bumps should typically be used for imo. i.e. commons v0.x supports Node 18 and commons v1.x supports Node 20. This is pretty much the standard process to my knowledge.

Yeah, if there are breaking changes then definitely. I also saw some packages doing a major anytime they increase node version (or min node version) but I'd probably just use minor in that case. Created api3dao/commons#38

@dcroote
Copy link
Contributor Author

dcroote commented Dec 7, 2023

One alternative could be to support both with:

"engines": {
    "node": "^18.14.0 || ^20.10.0",
    "pnpm": "^8.8.0"
  },

and update the lint-build-test Actions workflow to build & test both (e.g. matrix strategy).

I tried Node.js 20 and it installed and built fine. Four tests failed within processing.test.ts (example), but they call can be easily fixed with the same type of change:

-    await expect(throwingFunc).rejects.toEqual(new Error('SyntaxError: Unexpected identifier'));
+    await expect(throwingFunc).rejects.toThrow('SyntaxError: Unexpected identifier');

Even better- the change is backwards compatible to Node.js 18 so if incorporated, all tests pass for both versions.

@dcroote
Copy link
Contributor Author

dcroote commented Dec 12, 2023

Is the thumbs up reaction agreement? If so, I can go ahead and implement what I wrote in the comment here for api3dao/commons#38

@Siegrift
Copy link
Contributor

Siegrift commented Dec 12, 2023

Yeah. I've edited the previously created issue api3dao/commons#38 and assigned you.

@dcroote
Copy link
Contributor Author

dcroote commented Dec 15, 2023

@Siegrift - with api3dao/commons#40 merged, can you do a minor commons release in order for me to continue with this?

@Siegrift
Copy link
Contributor

@dcroote
Copy link
Contributor Author

dcroote commented Dec 20, 2023

I really do not understand what is happening with these E2E tests. Everything is fine with Node v18. Here with Node v20, the following happens with the 4 E2E suites that are run as part of the Actions matrix:

  • airnode-validator
    • passes locally
    • passes CI
  • airnode-admin
    • passes locally
    • fails in CI e.g. here
  • airnode-node
    • fails locally
    • fails in CI e.g. here
  • airnode-utilities
    • passes locally
    • passes CI (when it has the chance to run since often another of the E2E tests fails and in the Actions matrix format causes this to be cancelled)

The error is always

missing response (requestBody="{\"method\":\"eth_accounts\",\"params\":[],\"id\":93,\"jsonrpc\":\"2.0\"}", requestMethod="POST", serverError={"code":"ECONNRESET"}, url="http://127.0.0.1:8545/", code=SERVER_ERROR, version=web/5.7.1)

      at Logger.Object.<anonymous>.Logger.makeError (../../node_modules/@ethersproject/logger/src.ts/index.ts:269:28)
      at Logger.Object.<anonymous>.Logger.throwError (../../node_modules/@ethersproject/logger/src.ts/index.ts:281:20)
      at ../../node_modules/@ethersproject/web/src.ts/index.ts:292:28
      at step (../../node_modules/@ethersproject/web/lib/index.js:33:23)
      at Object.throw (../../node_modules/@ethersproject/web/lib/index.js:14:53)
      at rejected (../../node_modules/@ethersproject/web/lib/index.js:6:65)

It might have something to do with axios/axios#5929? Still investigating

dcroote and others added 8 commits January 27, 2024 17:28
axios
  * @api3/airnode-adapter: ^1.6.2 → ^1.6.7
hardhat
  * @api3/airnode-adapter: ^2.19.3 → ^2.19.5
  * @api3/airnode-examples: ^2.19.3 → ^2.19.5
  * @api3/airnode-operation: ^2.19.3 → ^2.19.5
  * @api3/airnode-protocol: ^2.19.3 → ^2.19.5
  * @api3/airnode-utilities: ^2.19.3 → ^2.19.5
@Siegrift
Copy link
Contributor

@dcroote Did you have a chance to try out Node@22 ?

@dcroote
Copy link
Contributor Author

dcroote commented May 15, 2024

I tried (locally) and same errors 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Node.js 18 with Node.js 20
3 participants